Factor in .gitignore for build cmd freshness

When testing the freshness of a build command, the fingerprint of an entire
package is generated. The current method of fingerprinting a path source is to
just stat() the entire tree and look at mtimes. This works fine when the build
command places *all* output in the build directory.

Some systems, like autotools, place *most* output in the build directory and
some output in the source directory, however (see spidermonkey). In this case
the coarse-grained "consider all files in a directory as input" is too painful
for the build command as the package will forever be rebuilt.

This commit adds support for filtering the list of files in a directory
considered to be part of a package by using `git ls-files`. This git-specific
logic can be extended to other VCSs when cargo grows support for them.

Closes #379

diff --git a/src/cargo/sources/path.rs b/src/cargo/sources/path.rs
index 8a6622d62081de4376373b4228359381ad186bbb..c7d0e7d5130ed87a049270364ff3ee67f55fa768 100644 (file)
--- a/src/cargo/sources/path.rs
+++ b/src/cargo/sources/path.rs
@@ -5,7 +5,7 @@ use std::io::fs;
 
 use core::{Package, PackageId, Summary, SourceId, Source, Dependency, Registry};
 use ops;
-use util::{CargoResult, internal, internal_error};
+use util::{CargoResult, internal, internal_error, process};
 
 pub struct PathSource {
     id: SourceId,
@@ -57,6 +57,74 @@ impl PathSource {
             ops::read_packages(&self.path, &self.id)
         }
     }
+
+    /// List all files relevant to building this package inside this source.
+    ///
+    /// This function will use the appropriate methods to determine what is the
+    /// set of files underneath this source's directory which are relevant for
+    /// building `pkg`.
+    ///
+    /// The basic assumption of this method is that all files in the directory
+    /// are relevant for building this package, but it also contains logic to
+    /// use other methods like .gitignore to filter the list of files.
+    pub fn list_files(&self, pkg: &Package) -> CargoResult<Vec<Path>> {
+        // TODO: add an `excludes` section to the manifest which is another way
+        // to filter files out of this set that is returned.
+        return if self.path.join(".git").exists() {
+            self.list_files_git(pkg)
+        } else {
+            self.list_files_walk(pkg)
+        };
+
+        fn list_files_git(&self, pkg: &Package) -> CargoResult<Vec<Path>> {
+            let cwd = pkg.get_manifest_path().dir_path();
+            let mut cmd = process("git").cwd(cwd.clone());
+            cmd = cmd.arg("ls-files").arg("-z");
+
+            // Filter out all other packages with a filter directive
+            for pkg in self.packages.iter().filter(|p| *p != pkg) {
+                if cwd.is_ancestor_of(pkg.get_manifest_path()) {
+                    let filter = pkg.get_manifest_path().dir_path()
+                                    .path_relative_from(&self.path).unwrap();
+                    cmd = cmd.arg("-x").arg(filter);
+                }
+            }
+
+            log!(5, "listing git files with: {}", cmd);
+            let output = try!(cmd.arg(".").exec_with_output());
+            let output = output.output.as_slice();
+            Ok(output.split(|x| *x == 0).map(Path::new).collect())
+        }
+
+        fn list_files_walk(&self, pkg: &Package) -> CargoResult<Vec<Path>> {
+            let mut ret = Vec::new();
+            for pkg in self.packages.iter().filter(|p| *p == pkg) {
+                let loc = pkg.get_manifest_path().dir_path();
+                try!(walk(&loc, &mut ret, true));
+            }
+            return Ok(ret);
+
+            fn walk(path: &Path, ret: &mut Vec<Path>,
+                    is_root: bool) -> CargoResult<()> {
+                if !path.is_dir() {
+                    ret.push(path.clone());
+                    return Ok(())
+                }
+                // Don't recurse into any sub-packages that we have
+                if !is_root && path.join("Cargo.toml").exists() { return Ok(()) }
+                for dir in try!(fs::readdir(path)).iter() {
+                    match (is_root, dir.filename_str()) {
+                        (_,    Some(".git")) |
+                        (true, Some("target")) |
+                        (true, Some("Cargo.lock")) => continue,
+                        _ => {}
+                    }
+                    try!(walk(dir, ret, false));
+                }
+                return Ok(())
+            }
+        }
+    }
 }
 
 impl Show for PathSource {
@@ -105,33 +173,14 @@ impl Source for PathSource {
         }
 
         let mut max = 0;
-
-        for pkg in self.packages.iter().filter(|p| *p == pkg) {
-            let loc = pkg.get_manifest_path().dir_path();
-            max = cmp::max(max, try!(walk(&loc, true)));
-        }
-
-        return Ok(max.to_string());
-
-        fn walk(path: &Path, is_root: bool) -> CargoResult<u64> {
-            if !path.is_dir() {
-                // An fs::stat error here is either because path is a
-                // broken symlink, a permissions error, or a race
-                // condition where this path was rm'ed - either way,
-                // we can ignore the error and treat the path's mtime
-                // as 0.
-                return Ok(fs::stat(path).map(|s| s.modified).unwrap_or(0))
-            }
-            // Don't recurse into any sub-packages that we have
-            if !is_root && path.join("Cargo.toml").exists() { return Ok(0) }
-
-            let mut max = 0;
-            for dir in try!(fs::readdir(path)).iter() {
-                if is_root && dir.filename_str() == Some("target") { continue }
-                if is_root && dir.filename_str() == Some("Cargo.lock") { continue }
-                max = cmp::max(max, try!(walk(dir, false)));
-            }
-            return Ok(max)
+        for file in try!(self.list_files(pkg)).iter() {
+            // An fs::stat error here is either because path is a
+            // broken symlink, a permissions error, or a race
+            // condition where this path was rm'ed - either way,
+            // we can ignore the error and treat the path's mtime
+            // as 0.
+            max = cmp::max(max, file.stat().map(|s| s.modified).unwrap_or(0));
         }
+        Ok(max.to_string())
     }
 }

diff --git a/tests/test_cargo_compile_git_deps.rs b/tests/test_cargo_compile_git_deps.rs
index e69cc0e75afa3f72aaabf1a849ac4231165dbbf6..cfb4c87d08e1d3ee22eaa246b80151e07cefd40b 100644 (file)
--- a/tests/test_cargo_compile_git_deps.rs
+++ b/tests/test_cargo_compile_git_deps.rs
@@ -1011,3 +1011,44 @@ test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured
 
 ", compiling = COMPILING, url = p.url(), running = RUNNING, bar = p2.url())));
 })
+
+test!(git_build_cmd_freshness {
+    let foo = git_repo("foo", |project| {
+        project.file("Cargo.toml", r#"
+            [package]
+            name = "foo"
+            version = "0.0.0"
+            authors = []
+            build = "true"
+        "#)
+        .file("src/lib.rs", "pub fn bar() -> int { 1 }")
+        .file(".gitignore", "
+            src/bar.rs
+            Cargo.lock
+        ")
+    }).assert();
+    foo.root().move_into_the_past().assert();
+
+    assert_that(foo.process(cargo_dir().join("cargo-build")),
+                execs().with_status(0)
+                       .with_stdout(format!("\
+{compiling} foo v0.0.0 ({url})
+", compiling = COMPILING, url = foo.url())));
+
+    // Smoke test to make sure it doesn't compile again
+    println!("first pass");
+    assert_that(foo.process(cargo_dir().join("cargo-build")),
+                execs().with_status(0)
+                       .with_stdout(format!("\
+{fresh} foo v0.0.0 ({url})
+", fresh = FRESH, url = foo.url())));
+
+    // Modify an ignored file and make sure we don't rebuild
+    println!("second pass");
+    File::create(&foo.root().join("src/bar.rs")).assert();
+    assert_that(foo.process(cargo_dir().join("cargo-build")),
+                execs().with_status(0)
+                       .with_stdout(format!("\
+{fresh} foo v0.0.0 ({url})
+", fresh = FRESH, url = foo.url())));
+})